-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for kafka 3 with zookeeper support (adding images for 3.0.1, 3.1.1 and 3.2.0) #709
base: master
Are you sure you want to change the base?
Conversation
# Conflicts: # .travis.yml # CHANGELOG.md # Dockerfile # README.md # test/0.10/test.create-topics.kafka.sh # test/docker-compose.yml
I would be happy to supply 3.0.1 support, but I think I would like @wurstmeister to comment a bit on the format of how the approach to the 3.x is done in this PR, is it ok to do it like this or should we do some other approach with the tests. are there new corner cases we need to test. |
… KAFKA_ADVERTISED_PORT KAFKA_PORT and KAFKA_HOST_NAME
@lunarfs I'm testing this out in my local environment. Looks pretty good except for a couple of issues if I have
So far so good after making those changes. |
Maybe |
…s alos where the internal-broker-list.sh is
…eflect this in tests, where it was not propperly mocked
@jimbogithub thanks, this was a nice catch, I did not properly modify the tests to catch this scenario.. I have modified tests such that i could capture the scenario. I think the BROKER_LIST is not a valid source for this, but the KAFKA_LISTNERS has the internal port captured, so getting it from there should be fine I think.. and then we also do not need to give extra params in the compose file as KAFKA_LISTNERS is already available in create-topics.sh. For the test container however BROKER_LIST is the better option, so if KAFKA_LISTNERS is not available we will use BROKER_LIST. |
The topic creation is still problematic. It's not working with the example config:
I use very similar config myself and it results in:
Maybe prefer |
@lunarfs @jimbogithub are there available any preview docker images of this change? |
@dswiecki I have an image at jimbodock/kafka:2.13-3.1.1. It is slightly stripped down (removed the embedded Docker which is only needed for the CI tests). It is based on the build at the time of #709 (comment) so supports |
🙏 Excited for this. |
OK, I will take a look at this one of the followin days |
🙏 Excited for this. |
🙏 Looking forward to this! |
@lunarfs I've added a PR to your PR that resolves the above by simply making the |
Preview available here: jimbodock/kafka:2.13-3.3.1. (with the embedded docker cli stripped out) |
|
Changed base image to eclipse-temurin
What is it good for
Support for Kafka 3.0.1, 3.1.1 and 3.2.0 with the regular ZooKeeper setup.
What I did
@Jack12816 did some approach of getting to kafka 3.0.0 I have worked with his commits as basis for getting to 3.0.1,2.1.1 and 3.2.0
the PR is inspired by Add support for kafka 2.8.1 #688 and Add support for Kafka 3.0.0. #691 I had to change some Kafka connection options (--zookeeper is deprecated since multiple versions, and since Kafka 3.0 it was removed -- replaced by the --bootstrap-server option. I did a fix for some tests for the new version/outputs
I have added code to report the no longer supported KAFKA_ADVERTISED_HOST_NAME KAFKA_ADVERTISED_PORT and KAFKA_PORT and KAFKA_HOST_NAME (see KAFKA-12945: Remove port, host.name and related configs in 3.0 apache/kafka#10872 for more information).
I have updated README.md to reflect that < 3.0 no longer supports KAFKA_ADVERTISED_HOST_NAME etc.
So i have done some changes to start-kafka.sh to reflect the removed options.. it is a syntactic nightmare of if then else.. and there might be some edge cases i have not yet covered.. feel free to comment
A picture of a cute animal (not mandatory but encouraged)